-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Program hash changed event #13
Added Program hash changed event #13
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey man, thanks for that! Stacked on #12 right?
src/config/component.cairo
Outdated
ProgramHashChanged { | ||
changed_by: get_caller_address(), | ||
old_program_hash: old_program_hash, | ||
new_program_hash: program_hash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in this contract the program info contains both the config hash and the program hash (as design that may be challenged @drspacemn if we should be more like solidity version), we should have a new event.
Something like:
struct ProgramInfoChanged {
changed_by: ContractAddress,
old_program_hash: ...,
new_program_hash: ...,
old_config_hash: ...,
new_config_hash: ...,
}
WDYT guys @Akashneelesh @drspacemn @b-j-roberts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep true, it will have both the program_hash and config_hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say either create a ProgramInfoChanged
event or add the ConfigHashChanged
event like Solidity and emit both events instead. Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'll just change it into ProgramInfoChanged and commit it now
Yess ser |
@@ -18,7 +18,7 @@ mod config_cpt { | |||
interface::IOwnable, | |||
}; | |||
use piltover::config::interface::IConfig; | |||
use starknet::ContractAddress; | |||
use starknet::{ContractAddress, get_caller_address}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our current style is to not import functions like this, instead when you use the function do Starknet::get_caller_address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is how its been done in other projects as well,
use starknet::{ContractAddress, get_caller_address};
I feel this is like a best practices thing, instead if we have another instance we dont need to do
Starknet::get_caller_address
Rather just call get_caller_address
src/config/component.cairo
Outdated
ProgramHashChanged { | ||
changed_by: get_caller_address(), | ||
old_program_hash: old_program_hash, | ||
new_program_hash: program_hash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say either create a ProgramInfoChanged
event or add the ConfigHashChanged
event like Solidity and emit both events instead. Up to you.
Implemented the event (ProgramHashChanged) for the function set_program_info